feat(cloudflare): Propagate traceparent to RPC calls - via fetch#19991
feat(cloudflare): Propagate traceparent to RPC calls - via fetch#19991
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Core
Other
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
3bd82cc to
71a3936
Compare
JPeer264
changed the title
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
71a3936 to
3097f54
Compare
FYI, #19960 landed and I agree! Right now, our various request instrumentations combine the "logic"/"algorithm" code with platform specific code how to set headers. I think it would be good if we could strip out the algorithm part into a common helper, and only leave the platform-specific parts to the individual instrumentations. From my PoV a good improvement but something we can also tackle at a later point. Happy to leave this up to you :) |
| } else if (!existing.split(',').some(entry => entry.trim().startsWith(SENTRY_BAGGAGE_KEY_PREFIX))) { | ||
| headers.set('baggage', `${existing},${baggage}`); | ||
| } |
There was a problem hiding this comment.
l: can we reuse mergeBaggageHeaders here?
Update: realized this is only exported from @sentry/node-core but I think that's actually worth moving to core because it should be fairly platform-independent. WDYT?
|
|
||
| if (!headers.has('sentry-trace')) { |
There was a problem hiding this comment.
m: As pointed out in #19960, we should only attach any of our headers if sentry-trace isn't set yet. So we could most likely just early return here.
| const newInit = addTraceHeaders(input, init); | ||
|
|
||
| return fetchFn(input, newInit); |
There was a problem hiding this comment.
m/q: is this how we add our tracing headers in our core/browser fetch instrumentation as well? Just wanted to double check because injecting headers into fetch arguments is a bit tricky with the various forms of input and init and the precedence.
I think adding headers to init is fine. Though us deep-copying headers from input to init, and then init again might be unexpected if one of the objects is reused by users across multiple requests. Anyway, not saying something is off but that we should double check that this is how we do it in the other SDKs as well.
There was a problem hiding this comment.
Fair enough, that would be indeed a bad behavior - I'll change the behavior to match the others
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
3097f54 to
fb6a835
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return init || {}; | ||
| } | ||
|
|
||
| const headers = new Headers(init?.headers || (input instanceof Request ? input.headers : undefined)); |
There was a problem hiding this comment.
Bug: The addTraceHeaders function drops headers from an incoming Request if the init object contains an empty but truthy Headers object, due to incorrect override logic.
Severity: HIGH
Suggested Fix
Modify addTraceHeaders to merge headers from both the input (if it's a Request) and init.headers. Instead of simply prioritizing init.headers, create a new Headers object from the input.headers first, and then append headers from init.headers to it. This ensures that headers from both sources are preserved.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/cloudflare/src/utils/addTraceHeaders.ts#L19
Potential issue: When `addTraceHeaders` is called with a `Request` object containing
headers and an `init` object with an empty `Headers` instance (e.g., `{ headers: new
Headers() }`), the function incorrectly prioritizes the empty headers. The logic at line
19 uses `init?.headers` if it's truthy, which an empty `Headers` object is.
Consequently, when the underlying `fetch` is called, the empty headers from `init`
override the original `Request` headers, causing critical headers like `Authorization`
to be silently dropped.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@Lms24 I just reworked the way how tracing headers are generated. I actually exported
|
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
77c7260 to
ef05c42
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-do/wrangler.jsonc
Show resolved
Hide resolved
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
3 participants
relates to #19327
related to #16898 (it is not really closing it as we just add context propagation without adding spans for individual calls. It needs to be defined if we need it)
It is important to know that these RPC calls do only work with the
.fetchcall:This adds RPC fetch calls between:
This works by instrumenting
env(viainstrumentEnv), which then goes over the bindings and see if there is a DurableObject or a normal Fetcher (full list of current bindings: https://developers.cloudflare.com/workers/runtime-apis/bindings/). This got inspired by howotel-cf-workersinstruments their env: https://github.com/evanderkoogh/otel-cf-workers/blob/effeb549f0a4ed1c55ea0c4f0d8e8e37e5494fb3/src/instrumentation/env.tsWith this PR I added a lot of tests to check if trace propagation works (so this PR might look like it added a lot of LoC, but it is mostly tests). So I added it for
scheduleandqueue, but it is not possible foremailandtailwithwrangler dev.Potential things to change
Trace propagagtion
I added the
addTraceHeaders.tshelper, as there is currently no way to reuse the existing logic (it is baked-in into the fetch instrumentations). It would be nice once #19960 lands that we can reuse it in Cloudflare to reuse existing code. I tried to write couple of tests so we don't have duplicated headers.Adding extra spans
So there is actually a guide by OTel to add RPC spans, but was talking with someone from the OTel maintainers and they meant that this wouldn't be necessary as we already have an
http.serverspan from out instrumented DurableObjects (and other resources) - so it wouldn't add much of information.Without RPC span:
With RPC span: